consoles: Allow configuring VNC #1973
Conversation
|
Can you add a screenshot of the current status please? Thanks! |
Yes! Thanks for having a look @garrett! A runing virtual machine looks and behaves exactly like before. The console card of a not running machine has details about VNC. Machine without VNC:
Machine with default VNC:
Machine with custom VNC:
Dialog for editing the VNC config:
Dialog for adding VNC:
(None of the above is meant to be final, of course.) |
mvollmer
force-pushed
the
console-vnc-edit
branch
from
2842ed0 to
cd06f95
Compare
|
Thanks for adding the screenshots! I like what I'm seeing so far.
It looks like a great start though, and I think it can be banged into resembling the mockups for this. Thanks again for picking up the work on Machines! |
I think I didn't see any mockups of a stopped machine, hmm. Is this mockup also meant for stopped machines?
|
mvollmer
force-pushed
the
console-vnc-edit
branch
3 times, most recently
from
f5583f6 to
78c6280
Compare
Sorry, that isn't true. A running machine without VNC now allows the selection of the VNC tab, which will look like this:
|
|
I have changed the information display a bit. It deemphasizes the "VNC" term and the listening address. No VNC:
VNC with auto port:
VNC with custom port:
In the selector, "VNC console" is changed to "Graphical console (VNC)".
The idea is that Cockpit talks about support for a graphical console inside Cockpit. That happens to be implemented via VNC, but that's a detail. And the per-VM listening address is not that interesting. People will very likely leave it at its default, and are better off changing the global default in qemu.conf in any case. The port on the other hand is interesting. VMs might fail to start if they use the same port by accident, and it is a genuine per-VM config setting. |
mvollmer
force-pushed
the
console-vnc-edit
branch
from
78c6280 to
5faf9fd
Compare
|
I have polished the dialogs some. They look like this now when everything is at default:
With a bogus port:
With a number but out of range:
|
mvollmer
force-pushed
the
console-vnc-edit
branch
from
0ceb441 to
59ed7bf
Compare
mvollmer
force-pushed
the
console-vnc-edit
branch
5 times, most recently
from
aa46133 to
9757a3a
Compare
|
Next round of screenshots! Shut-off machine without VNC:
"Add VNC server" dialog:
With a static port and a failed input validation:
Running machine with a serial console but no VNC yet:
Running machine with freshly added VNC and in need of a shutdown:
|
mvollmer
force-pushed
the
console-vnc-edit
branch
from
9757a3a to
1880020
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mvollmer
force-pushed
the
console-vnc-edit
branch
from
1880020 to
717459a
Compare
mvollmer
force-pushed
the
console-vnc-edit
branch
3 times, most recently
from
8d8def3 to
07dd919
Compare
- The dialogs talk about "server" and "listening" to make that clearer. - We use the empty string instead of "-1" to signify automatic port assignment in the UI. - The port does validation of its value. - The password field takes the whole row and has can reveal its value. - The dialogs warn if a shutdown is needed. - The components take the whole "vm" object instead of separate name, id, and connectionName.
We are not comfortable with that without also offering encryption.
mvollmer
force-pushed
the
console-vnc-edit
branch
3 times, most recently
from
9ea32bc to
ac20d90
Compare
- The "Console" card is always present and let's people manage VNC server settings while the machine is off. - The "VNC console" tab is always present when the machine is running and let's people add VNC. - There is no way yet to change VNC server settings for a running machine since there is no good place for the button that would open the dialog.
mvollmer
force-pushed
the
console-vnc-edit
branch
from
ac20d90 to
d74dafe
Compare
| if (!active_vnc) | ||
| return true; | ||
| if (inactive_vnc.port != -1 && active_vnc.port != inactive_vnc.port) | ||
| return true; |
There was a problem hiding this comment.
This added line is not executed by any test.
| if (inactive_vnc.port != -1 && active_vnc.port != inactive_vnc.port) | ||
| return true; | ||
| if (active_vnc.password != inactive_vnc.password) | ||
| return true; |
There was a problem hiding this comment.
This added line is not executed by any test.
| .catch(ex => onAddErrorNotification({ | ||
| text: cockpit.format(_("Failed to send key Ctrl+Alt+$0 to VM $1"), keyName, vmName), | ||
| text: cockpit.format(_("Failed to send key Ctrl+Alt+$0 to VM $1"), keyName, vm.name), |
There was a problem hiding this comment.
This added line is not executed by any test.
| detail: ex.message, | ||
| resourceId: vmId, | ||
| resourceId: vm.id, |
There was a problem hiding this comment.
This added line is not executed by any test.
| const { idPrefix, vm } = this.props; | ||
|
|
||
| const defaultBody = ( | ||
| <Form onSubmit={e => e.preventDefault()} isHorizontal> |
There was a problem hiding this comment.
This added line is not executed by any test.
| dialogError: undefined, | ||
| saveDisabled: false, | ||
| vncCustomPort: Number(props.consoleDetail.port) != -1, | ||
| vncPort: Number(props.consoleDetail.port) == -1 ? "" : props.consoleDetail.port || "", |
There was a problem hiding this comment.
This added line is not executed by any test.
| const { idPrefix, vm } = this.props; | ||
|
|
||
| const defaultBody = ( | ||
| <Form onSubmit={e => e.preventDefault()} isHorizontal> |
There was a problem hiding this comment.
This added line is not executed by any test.
There was a problem hiding this comment.
Thanks for your work on this, and we'll probably need this, but I don't think it's ready for merging.
It's not what we talked about, and we haven't heard back yet about the questions I asked (which were the questions we wondered about in person).
We need to talk about it and decide exactly how to move forward, which means we need a plan we can agree on, a design that we can target... and I can make some mockups on how it should look and act. But I need more information before working on designs and making mockups, and this needs to come both from an investigation into what is actually possible, and hearing back about what's actually needed and why.
Basically: We need to know what use cases we're actually trying to solve, address technical issues, possibly workaround limitations (as they _probably_still exist).
Again, I do greatly appreciate your effort (and we'll probably wind up using at least a big chunk of this code), but it needs more work on both ends (investigating, planning and designing upfront and then polishing at the end) before we merge in a change like this.
There was a problem hiding this comment.
Thanks! Sorry for the slightly chaotic comments, I was reviewing the commits individually -- but so far they should be squashed and really be reviewed as a whole.
I don't know what @garrett and you talked about, and I'm aware that we are currently reorganizing the console a lot -- but given how important that seems for the customer, we don't need to let the perfect be the enemy of the good. The general intention and workflows seem clear enough, and the limitation to "listening to localhost only" avoids all the encryption/firewall/security issues.
We may well have to extend this in the future (listening to public addresses), but: (1) starting with that is already an improvement, and (2) it's actually good to encourage ssh tunelling over all this madness of encryption, TLS setup, and firewall configuration for VNC. This is really not how things should be done in production. So as long as we can get away with it, I'd like to stay with this localhost setup as long as possible, and rather work on virt-viewer / .vv download to work with ssh tunneling.
I still found a bunch of inconsistencies and bugs, but most of them are hopefully quick to fix. Thanks for driving this @mvollmer !
| vncAddress: this.state.vncAddress || "", | ||
| vncPort: this.state.vncPort || "", | ||
| vncPassword: this.state.vncPassword || "", |
There was a problem hiding this comment.
Out of interest -- can these ever be null? Their state gets initialized as "", and you can't null-ify it via the PF widget. Neither the address nor the port can be 0. The password can be "0", but that's truthy. So overall this seems harmless.
| return ( | ||
| <Grid hasGutter md={6}> | ||
| <GridItem span={6}> | ||
| <FormGroup fieldId={`${idPrefix}-address`} label={_("VNC address")}> |
There was a problem hiding this comment.
The whole dialog is about VNC, so I think we should leave out the "VNC " prefix from the labels. That shortens them and avoids potential inconsistent translations. Plus, both "Address" and "Port" are already translated, so we'll get them for free.
| title={_("Edit VNC settings")} | ||
| footer={ | ||
| <> | ||
| <Button isDisabled={this.state.saveDisabled} id={`${idPrefix}-save`} variant='primary' onClick={this.save}> |
There was a problem hiding this comment.
saveDisabled is initialized to false and never changed. This was probably meant to be used with some form validation? But all fields are optional, right?
There was a problem hiding this comment.
In the next "consoles: Improve VNC dialogs" commit there's actually some input validation, but it still doesn't disable the Save button AFAICS.
| @@ -1089,3 +1089,29 @@ export async function domainAddTPM({ connectionName, vmName }) { | |||
| const args = ["virt-xml", "-c", `qemu:///${connectionName}`, "--add-device", "--tpm", "default", vmName]; | |||
| return cockpit.spawn(args, { err: "message", superuser: connectionName === "system" ? "try" : null }); | |||
| } | |||
|
|
|||
| export function domainAttachVnc({ connectionName, vmName, vncAddress, vncPort, vncPassword }) { | |||
There was a problem hiding this comment.
The two are almost identical, and so are the dialogs. Merging the dialogs is a bit more work, but the API can easily be merged by adding either an edit boolean or a mode string that supports "add" or "edit" (and throw on any other value).
| <> | ||
| <Grid hasGutter md={6}> | ||
| <GridItem span={6}> | ||
| <FormGroup fieldId={`${idPrefix}-address`} label={_("Listening address")}> |
There was a problem hiding this comment.
The "Listening" is redundant here, a bit jargony, and makes the label unnecessarily wide (translations will only aggravate this). The address wasn't part of your recent round of screenshots but of course we need it.
| vnc_info = _("not supported"); | ||
| vnc_action = ( | ||
| <Button variant="link" isInline onClick={add_vnc}> | ||
| {_("Add VNC")} |
There was a problem hiding this comment.
Should use the same string as above.
| <p> | ||
| { | ||
| vm.state == "running" | ||
| ? _("Shut down and restart the virtual machine to access the graphical console.") |
There was a problem hiding this comment.
s/Shut down and // to trim the sentence.
| { | ||
| vm.state == "running" | ||
| ? _("Shut down and restart the virtual machine to access the graphical console.") | ||
| : _("Please start the virtual machine to access its console.") |
There was a problem hiding this comment.
Again, let's make the sentences consistent. Use "its console" or "the graphical console" both here and in the line above.
| b.set_input_text("#add-vnc-port", "5000") | ||
| b.click("#add-vnc-add") |
There was a problem hiding this comment.
While this is type=text (see question above), this should also test a non-numeric value.
| ); | ||
| } else { | ||
| if (vnc.port == -1) | ||
| vnc_info = _("VNC, dynamic port"); |
There was a problem hiding this comment.
With a dynamic port it would be super helpful to actually show it, so that you know what to type into your virt-viewer. Can we get this from libvirt somehow? Is the port part of the downloaded .vv file?
| </FormHelperText> | ||
| } | ||
| </FormGroup> | ||
| <FormGroup fieldId={`${idPrefix}-password`} label={_("Password")}> |
There was a problem hiding this comment.
Does it actually make sense to offer a password for the "localhost only" use case? My gut feeling is "no", as that doesn't offer much protection in that use case. But @Shotaro-Kawaguchi should have the last word on that.
|
|
||
| return ( | ||
| <> | ||
| <FormGroup |
There was a problem hiding this comment.
With the dialog showing only the port, this could use an info alert/note that the server will only listen to localhost, and that you need to use ssh tunneling for remote access. If there's documentation about that somewhere, we should link to that. If not, let's not block on that, and people who don't know about that will then simply not use that feature -- they are no worse off than before.
|
Alright, in light of #1795 (comment), I propose to shelf this and replace it with a PR that only covers the "add VNC to a machine that is missing it" functionality. @Shotaro-Kawaguchi, I hope that works for you. |
Demo: https://youtu.be/02Gbc8OCA_M
Via the "Consoles" card, when the machine is off, but also via the VNC console tab when the machine is running and has no VNC.
This is the minimum to get #1795 finished, IMO, and we should not try to do more. Anything on top happens in #1972 and elsewhere, at its own pace.